Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add combine-based benchmark that caused a large performance regression #588

Closed
wants to merge 1 commit into from

Conversation

sdroege
Copy link

@sdroege sdroege commented Dec 22, 2019

@sdroege
Copy link
Author

sdroege commented Dec 22, 2019

This will probably time out on travis until the PR above is merged, it's taking quite a few minutes with latest nightly and after that PR is merged it will take about 10s.

@Mark-Simulacrum
Copy link
Member

We'll probably want to remove a few of the combine calls to get the post-PR runtime down to ~2-3 seconds at most; we try to keep the benchmarks in this repository very short as we run them lots of times while benchmarking (9 times minimum by default).

@sdroege
Copy link
Author

sdroege commented Dec 23, 2019

combine itself needs around 9s to build on my machine. The benchmark crate alone needs around 200ms with 1.40 and more than 20 minutes with current nightly.

@Mark-Simulacrum
Copy link
Member

The benchmark crate alone needs around 200ms with 1.40 and more than 20 minutes with current nightly.

Hm, so you originally said it takes around 10s after the PR -- does that mean there's still a regression vs. 1.40, or am I misinterpreting?

@sdroege
Copy link
Author

sdroege commented Dec 25, 2019

Hm, so you originally said it takes around 10s after the PR -- does that mean there's still a regression vs. 1.40, or am I misinterpreting?

Until rust-lang/rust#67454 is merged it (the benchmark crate itself, not its dependencies) will take quite a few minutes. With 1.40 and nightly after that is merged, combine itself takes about 9s on my machine and the benchmark crate itself 200ms on top of that.

@Mark-Simulacrum
Copy link
Member

Okay, so in that case, we're actually going to want to bump the runtime of the benchmark itself up a bit I guess? We want it in the 600-1500ms range I suspect.

@rylev
Copy link
Member

rylev commented Jul 9, 2021

This is pretty stale so I'm going to close for now. Feel free to reopen if you'd like to continue to work on it!

@rylev rylev closed this Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants